Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes twitch panel #2618

Merged
merged 1 commit into from
Jun 6, 2019
Merged

Fixes twitch panel #2618

merged 1 commit into from
Jun 6, 2019

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jun 6, 2019 •
edited by jasonrsadler
Loading

Resolves brave/brave-browser#3590
Resolves brave/brave-browser#4680
Resolves brave/brave-browser#3417

Submitter Checklist:

Test Plan:

  • enable rewards
  • go to https://twitch.tv/ilikeblueturtles
  • open panel and make sure that panel shows verified check, correct name and image
  • go to live streams and vods
  • make sure that correct data is displayed in the panel

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@NejcZdovc NejcZdovc added this to the 0.68.x - Nightly milestone Jun 6, 2019
@NejcZdovc NejcZdovc self-assigned this Jun 6, 2019
@NejcZdovc NejcZdovc added the CI/skip Do not run CI builds (except noplatform) label Jun 6, 2019
@NejcZdovc NejcZdovc force-pushed the twitch-fix branch 3 times, most recently from d453493 to 5f8a1c7 Compare June 6, 2019 07:02
@NejcZdovc NejcZdovc removed the CI/skip Do not run CI builds (except noplatform) label Jun 6, 2019
@NejcZdovc NejcZdovc force-pushed the twitch-fix branch 2 times, most recently from 0111e82 to 0c7792e Compare June 6, 2019 09:47
emerick
emerick reviewed Jun 6, 2019
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just had a few comments on it.

vendor/bat-native-ledger/src/bat/ledger/internal/media/twitch.cc Outdated
std::transform(media_id.begin(),
media_id.end(),
media_id.begin(),
::tolower);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use base::ToLowerASCII here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

vendor/bat-native-ledger/src/bat/ledger/internal/media/twitch.cc Outdated
return;
}
ledger::TwitchEventInfo old_event;
std::map<std::string, ledger::TwitchEventInfo>::const_iterator iter =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could use auto here to make this declaration more concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

vendor/bat-native-ledger/src/bat/ledger/internal/media/twitch.cc Outdated

if (publisher_id.empty()) {
const std::string url = publisher_url + "/videos";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like url isn't used anywhere, unless I'm just missing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're good on this:
https://github.com/brave/brave-core/pull/2618/files#diff-410ec7b37fbed59610d3617cce571df3R646

it's used outside of diff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@NejcZdovc NejcZdovc requested a review from emerick June 6, 2019 14:52
emerick
emerick approved these changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NejcZdovc NejcZdovc merged commit 26080ed into master Jun 6, 2019
@NejcZdovc NejcZdovc deleted the twitch-fix branch June 6, 2019 17:45
brave-builds pushed a commit that referenced this pull request Jun 7, 2019
@brave-browser-releases
Uplift of #2618 (squashed) to dev
94fe7c2
@brave-builds brave-builds mentioned this pull request Jun 7, 2019
Merged
4 tasks
@kjozwiak
Copy link
Member

Quickly went through the test case outlined under #2618 (comment) to make sure things are working before uplifting #2633. Looks like things are generally working. QA will eventually go through a more detailed check/verification. Example of the fix working under 0.68.35 Chromium: 75.0.3770.80:

Screen Shot 2019-06-11 at 10 19 09 AM

Screen Shot 2019-06-11 at 10 15 48 AM

Screen Shot 2019-06-11 at 10 11 57 AM

This was referenced Jun 11, 2019
Merged
Merged
This was referenced Jun 14, 2019
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonrsadler jasonrsadler jasonrsadler approved these changes

@emerick emerick emerick approved these changes

@ryanml ryanml Awaiting requested review from ryanml

Assignees

@NejcZdovc NejcZdovc

Labels
feature/rewards
Projects
None yet
Milestone
0.68.x - Release
4 participants
@NejcZdovc @kjozwiak @emerick @jasonrsadler